fix(agent-core/mcp): sanitize MCP JSON Schemas for Moonshot MFJS validator - #830
fix(agent-core/mcp): sanitize MCP JSON Schemas for Moonshot MFJS validator#830creatiVision wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c285628 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f20830c5c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Cherry-pick PR MoonshotAI#830's changes onto current upstream main, resolving the merge conflict in transformModelData (toml.ts) introduced by main's overrides-transform refactor: - schema.ts: add incompatible 'disabled' -> 'enabled' preprocess mapping - toml.ts: keep the overrides transform and add the max_tokens / max_output_tokens -> maxOutputSize alias fallback - Add sanitizeMcpSchema (MFJS emitter) + cycle detector + root-pointer comment addressing Codex review feedback on PR MoonshotAI#830 - Integrate the sanitizer into connectAndDiscoverTools - Update schema-sanitize export in index.ts - Add tests + changeset
6fc34e7 to
c359747
Compare
ca113cd to
03873ad
Compare
dd82ba7 to
a0fc9fe
Compare
|
Addressed the three Codex review notes on
Tests: |
6d335a7 to
797107d
Compare
797107d to
a2da2c3
Compare
🔄 Status Update & Necessity NoteRebased cleanly onto latest Why this PR is still necessary:
This PR provides lightweight, deterministic schema sanitization during MCP tool discovery so all standard MCP tools work out-of-the-box. |
fb498a3 to
10dc44d
Compare
51ab3ef to
04dba24
Compare
…ap disabled config
b33fe94 to
b759414
Compare
|
Rebased and updated onto latest
All merge conflicts are resolved and the PR is clean, mergeable, and ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c285628f9e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| name: mcpTool.name, | ||
| description: mcpTool.description, | ||
| parameters: assertMcpInputSchema(mcpTool.name, mcpTool.inputSchema), | ||
| parameters: sanitizeMcpSchema(assertMcpInputSchema(mcpTool.name, mcpTool.inputSchema)), |
There was a problem hiding this comment.
Keep Kimi-only normalization out of MCP discovery
When using Anthropic, OpenAI, or Google with an MCP tool, this stores the Kimi-specific rewrite in the shared Tool.parameters; those request formatters and the tool-argument validator subsequently consume the rewritten schema unchanged. Valid schemas such as a typeless arbitrary-value property are therefore narrowed to string, and tuple schemas are altered even though those providers can accept the original schema, causing models to generate or local validation to reject otherwise valid arguments. Preserve the MCP schema here and apply this compatibility normalization only in the Kimi request path.
Useful? React with 👍 / 👎.
| } else if (items.length === 1) { | ||
| record['items'] = items[0] as Json; | ||
| } else { | ||
| record['items'] = { anyOf: items }; |
There was a problem hiding this comment.
Preserve tuple positions when replacing array-form items
For a draft-07 tuple such as items: [{type: 'number'}, {type: 'string'}], replacing items with a single anyOf schema allows either type at every position instead of preserving each positional constraint. It also makes an accompanying additionalItems: false ineffective, so extra elements become valid. This can let the model and local argument validator accept calls the MCP server rejects; convert to an equivalent positional representation rather than collapsing the branches.
Useful? React with 👍 / 👎.
| const record = node as JsonRecord; | ||
| if (typeof record['$ref'] === 'string') { | ||
| const ref = record['$ref']; | ||
| if (ref.startsWith('#')) { |
There was a problem hiding this comment.
Resolve or preserve plain-name fragment references
When a valid modern JSON Schema uses $anchor: 'node' and $ref: '#node', this condition treats the plain-name fragment as a JSON Pointer; resolvePointer then looks for a top-level node property and throws if it is absent. Because sanitization runs during discovery, one such tool marks the entire MCP connection failed, including for non-Kimi providers. Restrict pointer resolution to # and #/..., or implement $anchor lookup and preserve unsupported fragments.
Useful? React with 👍 / 👎.
Related Issue
Resolves #792, #1610, #2661.
Related / config fixes: #2421, #302.
Continues / supersedes the earlier drafts in #1605 and #2216.
Problem
MCP servers often advertise standard JSON Schema tool parameters. Moonshot’s stricter MFJS validator (and Kimi’s discovery path) reject or crash on several common shapes:
$ref→ stack overflow during processingtypeon properties (enum/structure-only schemas) →type is not defineditemsarrays →items must be an object[1, "auto"]) must not be collapsed to a single forcedtypetypealongsideanyOf/oneOf→not a valid moonshot flavored json schemaWhat changed
sanitizeMcpSchemaand run it after MCP tool discovery inMcpConnectionManager(packages/agent-core/src/mcp/).$refinlining with RFC 6901 pointer unescaping (~1/~0) and circular-safe placeholders.typefilling from enum/const/structure.items→prefixItems+minItems/maxItems(preserves positional validation).anyOfbranches (preserves all members).disabled→enabled,max_tokens/max_output_tokens→maxOutputSize(Bug: [tools].disabled in config.toml does not filter MCP tools from the request #2421).packages/agent-core/test/mcp/schema-sanitize.test.ts, all 75 tests green).Checklist
typenext toanyOf#2661, Bug: [tools].disabled in config.toml does not filter MCP tools from the request #2421)schema-sanitize, config-loader, configs)main